Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

전남대 FE_이도현_2주차 과제 #19

Open
wants to merge 11 commits into
base: leedyun
Choose a base branch
from

Conversation

leedyun
Copy link

@leedyun leedyun commented Jul 4, 2024

base repository 변경을 안해서 다시보냅니다!


import React from 'react';

const Footer: React.FC = () => (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컴포넌트 스타일링은 따로 안하셨을까요 ?

}

const GiftRanking: React.FC = () => {
const [filter, setFilter] = useState<string>('all');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter 타입이 정해져있는데 string 보다는 type 으로 지정하는게 type narrowing 측면에서 낫습니다.


const GiftRanking: React.FC = () => {
const [filter, setFilter] = useState<string>('all');
const [subFilter, setSubFilter] = useState<string>('wanted');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subFilter 의 타입도 마찬가지 입니다.

{['all', 'women', 'men', 'teenagers'].map((category) => (
<div key={category} className={`button-container ${filter === category ? 'active' : ''}`} onClick={() => handleFilterChange(category)}>
<button>
<span>{category === 'all' ? 'ALL' : category === 'women' ? '👩🏻‍🦳' : category === 'men' ? '👨🏻‍🦳' : '👦🏻'}</span>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 삼항 연산자는 가독성을 떨어뜨립니다.

객체로 이를 대체할 수 있습니다.

// e.g.
const CATEGORY = {
  all: 'ALL',
  women: '👩🏻‍🦳',
  men: '👨🏻‍🦳',
};

 <span>{CATEGORY[category] || '👦🏻'}</span>

<button>
<span>{category === 'all' ? 'ALL' : category === 'women' ? '👩🏻‍🦳' : category === 'men' ? '👨🏻‍🦳' : '👦🏻'}</span>
</button>
<span className="category">{category === 'all' ? '전체' : category === 'women' ? '여성이' : category === 'men' ? '남성이' : '청소년이'}</span>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 마찬가지 입니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백해주셔서 감사합니다.
말씀하신 부분은 수정 완료했습니다.
GiftRanking 이외에 부분은 따로 수정할 부분은 없을까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백해주셔서 감사합니다. 말씀하신 부분은 수정 완료했습니다. GiftRanking 이외에 부분은 따로 수정할 부분은 없을까요?

추가로 남겨드린 피드백만 반영해주시면 따로 없을거 같아요 !

refactor : filter와 subFilter 타입을 string에서 타입으로 변경

refactor : 카테고리와 서브카테고리 라벨을 객체로 대체

return (
<>
<Header />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header, Footer 등 반복되는 컴포넌트는 Layout 으로 구성할 수 있을거 같습니다.
아래 참고 하셔서 Layout 컴포넌트로 구성해보실 수 있을거 같아요.
ref. https://reactrouter.com/en/main/components/outlet

);
};

export default App;
const ApplicationRoutes = () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routes 는 별도의 컴포넌트로 작성해서 App.tsx 에서 import 하도록 변경하는게 가독성 측명에서 더 나을거 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants